Feat: Usage library #1
Conversation
- Database adapter - ClickHouse adapter
WalkthroughAdds a Utopia Usage library: a pluggable Adapter interface and implementations for ClickHouse and Database, an abstract SQL adapter base, Metric model, Query builder, and a Usage facade that delegates to adapters. Introduces Dockerfile and docker-compose for MariaDB/ClickHouse/usage, PHPUnit config, extensive unit and integration tests and test traits, composer updates (autoload, dev deps, scripts), documentation (README, CONTRIBUTING, CODE_OF_CONDUCT, LICENSE), Pint config, and GitHub Actions workflows for tests, linter, and CodeQL. Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
lohanidamodar
requested review from
eldadfux and
shimonewman
and removed request for
eldadfux and
shimonewman
src/Usage/Adapter/ClickHouse.php
Outdated
| * | ||
| * @throws Exception | ||
| */ | ||
| public function log(string $metric, int $value, string $period = '1h', array $tags = []): bool |
There was a problem hiding this comment.
We can use constants for the periods
| 'tags' => json_encode($tags), | ||
| ]; | ||
|
|
||
| if ($this->sharedTables) { |
There was a problem hiding this comment.
Why are we aware for the concept of shared vs not shared tables in this scope? We can completely abstract this.
There was a problem hiding this comment.
Abstract this where? for database library, Database already abstracts it, but for Clickhouse we don't have a separate library, either we create a clickhouse client that handles these stuff, that can be shared here and in audits as well, or we duplicate this logic both here in audit library in clickhouse adapter
| $tableName = $this->getTableName(); | ||
| $escapedDatabaseAndTable = $this->escapeIdentifier($this->database) . '.' . $this->escapeIdentifier($tableName); | ||
|
|
||
| $sql = " |
There was a problem hiding this comment.
how are we protecting against SQL injections here?
src/Usage/Adapter/ClickHouse.php
Outdated
| foreach ($metrics as $metricData) { | ||
| $period = $metricData['period'] ?? '1h'; | ||
|
|
||
| if (! isset(self::PERIODS[$period])) { |
There was a problem hiding this comment.
I would add better validation here of the structure we expect. Might even introduce a Metric class
…pdate tests for new behavior
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@tests/Usage/UsageBase.php`:
- Around line 149-167: The test testIncrementingDefaultBehavior uses
assertEventually with a 2-second timeout which is too short for ClickHouse
merges; update the test to use a longer timeout (e.g., 10s) when running against
ClickHouse (or increase the default timeout) so the two logged increments can be
merged before assertions; locate testIncrementingDefaultBehavior and the call to
$this->assertEventually(..., 2) and change the timeout or add an adapter check
(e.g., detect ClickHouseTest / adapter type) to use a longer wait only for
ClickHouse.
- Around line 101-113: ClickHouse's SummingMergeTree can leave rows unmerged
causing flaky counts in testGetByPeriod; fix by forcing a synchronous merge and
slightly increasing wait: after calling $this->usage->setup() in
ClickHouseTest::setUp(), run an OPTIMIZE TABLE ... FINAL for the relevant table
to flush merges, and also override testGetByPeriod() in ClickHouseTest to call
assertEventually(..., timeout: 10) instead of the default 5s; this ensures rows
aggregated by buildDeterministicId() (see SQL.php) are merged before assertions.
🧹 Nitpick comments (3)
src/Usage/Adapter/SQL.php (1)
74-89: Add return type hint for consistency.The
getAttribute()method lacks a nullable return type hint in the signature, though the PHPDoc correctly documents it.♻️ Suggested improvement
- protected function getAttribute(string $id) + protected function getAttribute(string $id): ?array {tests/Usage/UsageBase.php (1)
186-203: Consider reducing sleep durations in purge test.The test uses
sleep(2)twice (4 seconds total), which slows down the test suite. If the sleep is only needed to ensure time progression for the purge threshold, consider using a fixed past timestamp instead.♻️ Suggested improvement
public function testPurge(): void { - sleep(2); - // Add a metric $this->usage->log('purge-test', 999, '1h'); - // Wait a bit - sleep(2); + // Small delay to ensure metric is persisted + usleep(500_000); // Purge all metrics $status = $this->usage->purge(DateTime::now());src/Usage/Adapter/Database.php (1)
193-204: Consider using database-level SUM aggregation for efficiency.The current implementation fetches all matching documents and sums in PHP. For large datasets, a database-level aggregation would be more efficient. However, this may be acceptable if the Utopia Database library doesn't expose aggregation functions.
| public function testIncrementingDefaultBehavior(): void | ||
| { | ||
| // Ensure clean state | ||
| $this->usage->purge(\Utopia\Database\DateTime::now()); | ||
|
|
||
| // Log the same metric twice with identical period and tags | ||
| $this->assertTrue($this->usage->log('increment-test', 5, '1h', [])); | ||
| $this->assertTrue($this->usage->log('increment-test', 7, '1h', [])); | ||
| $this->assertEventually(function () { | ||
| // Because adapters now aggregate by deterministic id/time/period (and tenant where applicable), | ||
| // there should be a single record and the summed value should be 12. | ||
| $results = $this->usage->getByPeriod('increment-test', '1h'); | ||
| $this->assertEquals(1, count($results)); | ||
|
|
||
| $sum = $this->usage->sumByPeriod('increment-test', '1h'); | ||
| $this->assertEquals(12, $sum); | ||
|
|
||
| }, 2); | ||
| } |
There was a problem hiding this comment.
Pipeline failure: Same eventual consistency issue as testGetByPeriod.
Line 161 fails in ClickHouseTest. The 2-second timeout may be insufficient for ClickHouse background merges. This test logs the same metric twice and expects a single aggregated row.
🧰 Tools
🪛 GitHub Actions: Tests
[error] 161-161: Utopia\Tests\Adapter\ClickHouseTest::testIncrementingDefaultBehavior: Failed asserting that 2 matches expected 1.
🤖 Prompt for AI Agents
In `@tests/Usage/UsageBase.php` around lines 149 - 167, The test
testIncrementingDefaultBehavior uses assertEventually with a 2-second timeout
which is too short for ClickHouse merges; update the test to use a longer
timeout (e.g., 10s) when running against ClickHouse (or increase the default
timeout) so the two logged increments can be merged before assertions; locate
testIncrementingDefaultBehavior and the call to $this->assertEventually(..., 2)
and change the timeout or add an adapter check (e.g., detect ClickHouseTest /
adapter type) to use a longer wait only for ClickHouse.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/Usage/Adapter/ClickHouse.php`:
- Around line 53-55: The default value of the private property $useFinal in
ClickHouse.php is currently true which enables FINAL for all queries; change its
default to false to avoid forcing merge-on-read in production and update the doc
comment accordingly, and ensure any tests that require FINAL call the existing
setUseFinal(true) method (or add one if missing) to enable it explicitly.
| /** @var bool Whether to use FINAL in SELECT queries to force merge-on-read (tests) */ | ||
| private bool $useFinal = true; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
ClickHouse FINAL clause performance impact production
💡 Result:
In ClickHouse, FINAL is a query-time “force merge” for certain MergeTree-family engines (notably ReplacingMergeTree, CollapsingMergeTree, SummingMergeTree, AggregatingMergeTree, etc.). It makes the query apply the same transformation/dedup/collapse logic that would normally happen during background merges, but at read time. [1]
Production performance impact (what to expect)
- Higher CPU + memory per query: the merge/collapse/dedup work is done in-memory during query execution instead of asynchronously in the background. [1]
- More data read / extra columns read:
FINALqueries may need to read primary key columns even if you didn’t select them, increasing IO and work. [1] - Can become “much slower” on wide scans: overhead is most noticeable when the query is not selective on the primary key, because more rows must be loaded and deduplicated. [3]
- Optimization gap in a common case: for
ReplacingMergeTree, if yourWHEREdoes not use key columns, ClickHouse “does not currently utilizePREWHEREoptimization when usingFINAL”, which can further increase rows read and slow queries. [3] - Concurrency is bounded:
FINALexecutes in parallel, but is limited bymax_final_threads(so it can saturate CPU on busy nodes if manyFINALqueries run). [1]
When it’s acceptable vs risky
- Acceptable (common production pattern): occasional correctness-critical reads (e.g., dedup-needed queries on
ReplacingMergeTree)—especially when filtered by primary key / ordered key, so the dedup set is small. [1][3] - Risky: putting
FINALon high-QPS dashboards, unbounded scans, or large ranges—it can shift heavy background work into peak query time and hurt latency/throughput. [1][3]
Practical ways teams reduce/avoid FINAL in production
- Rely on background merges for eventual correctness where possible, and reserve
FINALfor the few queries that truly need immediate dedup/collapse correctness. [1][4] - Make
WHEREselective on the table’s key (or restructure queries so they are), to cut the amount of data that must be deduplicated underFINAL. [3] - Partition so “same logical row” updates land in the same partition (partition key must not change per row), reducing the scope of merge work. [3]
Don’t confuse with OPTIMIZE TABLE ... FINAL
OPTIMIZE ... FINAL is a storage-level forced merge and is generally warned against in most cases because it can be very resource-intensive and impact cluster performance; it’s different from query-time FINAL. [2][4]
Sources: ClickHouse docs on FINAL behavior/drawbacks and settings [1], ClickHouse guidance on avoiding OPTIMIZE FINAL [2], ReplacingMergeTree guide section on FINAL performance and PREWHERE limitation [3], ClickHouse KB on OPTIMIZE FINAL vs FINAL [4].
🏁 Script executed:
# Look at the ClickHouse adapter file to understand context and usage
head -100 src/Usage/Adapter/ClickHouse.php | cat -nRepository: utopia-php/usage
Length of output: 3707
🏁 Script executed:
# Search for usage of useFinal in the codebase
rg "useFinal" -B 2 -A 2Repository: utopia-php/usage
Length of output: 2114
🏁 Script executed:
# Search for table engine type being used
rg -i "mergeTree|engine|table" src/Usage/Adapter/ClickHouse.php -B 1 -A 1 | head -50Repository: utopia-php/usage
Length of output: 1603
Default useFinal to false to avoid production performance degradation.
ClickHouse's FINAL clause forces merge-on-read, shifting expensive dedup/collapse work from background merges to query time. For high-volume metrics queries, this significantly increases CPU and memory per query. While the comment indicates FINAL is intended for tests, the default of true applies to all production code. Tests should explicitly call setUseFinal(true) when needed; production should rely on background merges per ClickHouse best practices.
Proposed fix
- private bool $useFinal = true;
+ private bool $useFinal = false;🤖 Prompt for AI Agents
In `@src/Usage/Adapter/ClickHouse.php` around lines 53 - 55, The default value of
the private property $useFinal in ClickHouse.php is currently true which enables
FINAL for all queries; change its default to false to avoid forcing
merge-on-read in production and update the doc comment accordingly, and ensure
any tests that require FINAL call the existing setUseFinal(true) method (or add
one if missing) to enable it explicitly.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/Usage/UsageBase.php`:
- Around line 86-93: The testGetBetweenDates assertion is a no-op because
assertGreaterThanOrEqual(0, count($results)) always passes; update the assertion
to verify meaningful behavior from $this->usage->getBetweenDates('requests',
$start, $end) — e.g. assert that the results are not empty (assertNotEmpty or
assertGreaterThan(0, count($results))) and optionally assertIsArray($results)
and that at least one item contains expected keys/metrics (e.g. a 'value' or
timestamp) based on the seeded '1h' metrics so the test fails when no data is
returned.
🧹 Nitpick comments (2)
tests/Usage/UsageBase.php (2)
67-72: Potential ClickHouse flakiness due to eventual consistency.Similar to
testGetByPeriod, ClickHouse'sSummingMergeTreemerges rows asynchronously. The assertion on line 71 may intermittently fail if rows haven't merged yet when running against ClickHouse.Consider adding eventual consistency handling (e.g., retry loop with timeout) for ClickHouse adapter tests, consistent with fixes applied to other tests in this trait.
150-167: Consider reducing sleep duration or documenting necessity.The test uses
sleep(2)twice, adding 4 seconds to the test suite runtime. While some delay may be necessary for time-based purge testing, consider:
- Reducing the sleep duration if shorter intervals suffice
- Adding a brief comment explaining why these sleeps are required
| public function testGetBetweenDates(): void | ||
| { | ||
| $start = DateTime::addSeconds(new \DateTime(), -3600); // 1 hour ago | ||
| $end = DateTime::now(); | ||
|
|
||
| $results = $this->usage->getBetweenDates('requests', $start, $end); | ||
| $this->assertGreaterThanOrEqual(0, count($results)); | ||
| } |
There was a problem hiding this comment.
Assertion is effectively a no-op.
count($results) always returns a non-negative integer, so assertGreaterThanOrEqual(0, ...) will always pass. This test doesn't verify any meaningful behavior.
Consider asserting that results are actually returned given the seeded '1h' metrics:
Proposed fix
public function testGetBetweenDates(): void
{
$start = DateTime::addSeconds(new \DateTime(), -3600); // 1 hour ago
$end = DateTime::now();
$results = $this->usage->getBetweenDates('requests', $start, $end);
- $this->assertGreaterThanOrEqual(0, count($results));
+ $this->assertGreaterThan(0, count($results), 'Expected at least one result for seeded 1h metrics');
}🤖 Prompt for AI Agents
In `@tests/Usage/UsageBase.php` around lines 86 - 93, The testGetBetweenDates
assertion is a no-op because assertGreaterThanOrEqual(0, count($results)) always
passes; update the assertion to verify meaningful behavior from
$this->usage->getBetweenDates('requests', $start, $end) — e.g. assert that the
results are not empty (assertNotEmpty or assertGreaterThan(0, count($results)))
and optionally assertIsArray($results) and that at least one item contains
expected keys/metrics (e.g. a 'value' or timestamp) based on the seeded '1h'
metrics so the test fails when no data is returned.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/Usage/Adapter/ClickHouse.php`:
- Around line 424-427: The tenant column is declared as 'tenant
Nullable(UInt64)' while the table ORDER BY uses (tenant, id), which breaks
MergeTree since nullable keys are disallowed; change the column definition in
the columns array (where $this->sharedTables adds 'tenant Nullable(UInt64)') to
a non-nullable type with a sentinel default (e.g., 'tenant UInt64 DEFAULT 0')
and update any tenant value assignment logic elsewhere to insert 0 when no
tenant is present; alternatively, if you prefer the other approach, add
"SETTINGS allow_nullable_key = 1" to the table creation settings instead (less
recommended for performance).
- Around line 749-763: The validation block for the tenant field mistakenly
reads $metricData['$tenant'] instead of $metricData['tenant'], causing the wrong
key to be accessed; update the access in the code that assigns $tenantValue (the
variable $tenantValue assigned from $metricData) to use 'tenant' (no dollar
sign), keeping the existing checks (is_int, ctype_digit, negative check and
exception messages) intact so the non-negative integer validation on
$tenantValue works as intended.
| if (array_key_exists('tenant', $metricData)) { | ||
| $tenantValue = $metricData['$tenant']; | ||
|
|
||
| if ($tenantValue !== null) { | ||
| if (is_int($tenantValue)) { | ||
| if ($tenantValue < 0) { | ||
| throw new Exception("Metric #{$index}: 'tenant' cannot be negative"); | ||
| } | ||
| } elseif (is_string($tenantValue) && ctype_digit($tenantValue)) { | ||
| // ok numeric string | ||
| } else { | ||
| throw new Exception("Metric #{$index}: 'tenant' must be a non-negative integer, got " . gettype($tenantValue)); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Tenant key mismatch causes validation to access wrong array key.
Line 749 checks for 'tenant' but line 750 accesses '$tenant' (with dollar sign prefix). This means validation passes when 'tenant' exists, but then reads from the non-existent '$tenant' key, likely returning null and bypassing the intended validation.
🐛 Proposed fix
// Validate tenant when provided (metric-level tenant overrides adapter tenant)
- if (array_key_exists('tenant', $metricData)) {
- $tenantValue = $metricData['$tenant'];
+ if (array_key_exists('$tenant', $metricData)) {
+ $tenantValue = $metricData['$tenant'];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (array_key_exists('tenant', $metricData)) { | |
| $tenantValue = $metricData['$tenant']; | |
| if ($tenantValue !== null) { | |
| if (is_int($tenantValue)) { | |
| if ($tenantValue < 0) { | |
| throw new Exception("Metric #{$index}: 'tenant' cannot be negative"); | |
| } | |
| } elseif (is_string($tenantValue) && ctype_digit($tenantValue)) { | |
| // ok numeric string | |
| } else { | |
| throw new Exception("Metric #{$index}: 'tenant' must be a non-negative integer, got " . gettype($tenantValue)); | |
| } | |
| } | |
| } | |
| if (array_key_exists('$tenant', $metricData)) { | |
| $tenantValue = $metricData['$tenant']; | |
| if ($tenantValue !== null) { | |
| if (is_int($tenantValue)) { | |
| if ($tenantValue < 0) { | |
| throw new Exception("Metric #{$index}: 'tenant' cannot be negative"); | |
| } | |
| } elseif (is_string($tenantValue) && ctype_digit($tenantValue)) { | |
| // ok numeric string | |
| } else { | |
| throw new Exception("Metric #{$index}: 'tenant' must be a non-negative integer, got " . gettype($tenantValue)); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@src/Usage/Adapter/ClickHouse.php` around lines 749 - 763, The validation
block for the tenant field mistakenly reads $metricData['$tenant'] instead of
$metricData['tenant'], causing the wrong key to be accessed; update the access
in the code that assigns $tenantValue (the variable $tenantValue assigned from
$metricData) to use 'tenant' (no dollar sign), keeping the existing checks
(is_int, ctype_digit, negative check and exception messages) intact so the
non-negative integer validation on $tenantValue works as intended.
…tamps and improve deterministic ID generation
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/Usage/Adapter/ClickHouse.php`:
- Around line 1085-1097: The case label uses the non-existent constant
Query::TYPE_SELECT causing a runtime error; update the switch to use
Query::TYPE_IN instead (replace Query::TYPE_SELECT with Query::TYPE_IN) so the
IN-value handling block (the code that calls validateAttributeName,
escapeIdentifier, formatParamValue and appends to $filters with an IN (...)
clause) matches the correct Query constant.
In `@src/Usage/Adapter/Database.php`:
- Around line 66-70: The code sets $time to a sentinel string for period 'inf'
in the log path but uses null in logBatch, causing inconsistent IDs; update both
places (the $time assignment in the log() method and the analogous assignment in
logBatch()) to use null when $period === 'inf' (i.e., set $time = $period ===
'inf' ? null : $now->format(Usage::PERIODS[$period])) and ensure any DB insert
logic that writes $time accepts null consistently so log() and logBatch()
produce the same bucket value.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/Usage/Adapter/Database.php`:
- Around line 184-189: The IN branch currently maps Query::TYPE_IN to
DatabaseQuery::contains($attribute, $values), which produces substring/element
matching; change it to use DatabaseQuery::equal($attribute, $values) so
Query::in(...) yields exact IN semantics for scalar columns: locate the switch
handling Query::TYPE_IN in Database.php and replace the call to
DatabaseQuery::contains with DatabaseQuery::equal, preserving the $attribute and
$values usage and the surrounding typing comment.
In `@src/Usage/Query.php`:
- Around line 83-93: The equal() method currently requires array $value which
breaks the established pattern; change Query::equal to accept mixed $value and
internally normalize it to an array (e.g., if not an array, wrap into [$value])
before constructing the Query, and update its implementation accordingly; then
remove the manual wrapping in the ClickHouse code that currently passes
[$metric] to Query::equal so values are not double-wrapped (look for the places
in ClickHouse where equal() is called and adjust those call sites).
🧹 Nitpick comments (6)
src/Usage/Usage.php (1)
60-63: Use constant for default period value.The default parameter uses the string literal
'1h'instead ofself::PERIOD_1H. This creates an inconsistency since the constant is defined and used elsewhere.Suggested fix
- public function log(string $metric, int $value, string $period = '1h', array $tags = []): bool + public function log(string $metric, int $value, string $period = self::PERIOD_1H, array $tags = []): booltests/Usage/UsageBase.php (1)
150-167: Consider reducing sleep duration or using polling for faster tests.The test uses
sleep(2)twice (4 seconds total), making it slow. Consider reducing the sleep durations or implementing a retry/polling mechanism to speed up the test suite while still ensuring the purge behavior is validated.Potential improvement
public function testPurge(): void { - sleep(2); + sleep(1); // Add a metric $this->usage->log('purge-test', 999, '1h'); // Wait a bit - sleep(2); + sleep(1); // Purge all metrics $status = $this->usage->purge(DateTime::now());src/Usage/Adapter/ClickHouse.php (1)
424-427: Nullable tenant column withallow_nullable_key=1is functional but suboptimal.The
tenantcolumn isNullable(UInt64)and used inORDER BYwithallow_nullable_key = 1setting. While this works, using a non-nullable type with a sentinel default (e.g.,UInt64 DEFAULT 0) would be more performant for ClickHouse query optimization.Also applies to: 450-459
src/Usage/Adapter/Database.php (2)
55-59: Suppress unused parameter warning.The
$idparameter is required by the abstractSQLparent class but unused in this adapter. Add a suppression annotation to silence the static analysis warning.Suggested fix
/** * Get column definition for Database adapter (not used, but required by SQL parent) + * + * `@SuppressWarnings`(PHPMD.UnusedFormalParameter) */ protected function getColumnDefinition(string $id): string {
270-281: Inefficient sum implementation fetches all rows.
sumByPeriodretrieves all matching documents and iterates in PHP to compute the sum. For large datasets, this is memory-intensive. Consider using database-level aggregation ifUtopia\Databasesupports it, or document this limitation.Consider using database sum if available
// If Utopia\Database supports sum aggregation: // return $this->db->sum($this->collection, 'value', $dbQueries); // Otherwise, document the limitation and consider pagination // for very large result sets to avoid memory issuessrc/Usage/Query.php (1)
5-10: Update class docblock - not just for ClickHouse.The docblock states "Query class for ClickHouse Audit adapter" but this class is used by both ClickHouse and Database adapters. Update the description to reflect its general purpose.
Suggested fix
/** - * Query class for ClickHouse Audit adapter + * Query class for Usage adapters * - * Provides a fluent interface for building ClickHouse queries. - * Contains only methods needed for audit log operations. + * Provides a fluent interface for building usage metric queries. + * Used by all adapter implementations (Database, ClickHouse, etc.) */
…ods; update ClickHouse and Database adapters to support new query types and improve handling of array values
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/Usage/Adapter/ClickHouse.php`:
- Around line 1016-1157: The CodeQL issue is that Query::getValues() always
returns an array, so all is_array($values) conditional branches are dead; update
the switch handling in ClickHouse.php (cases like Query::TYPE_EQUAL,
TYPE_LESSER, TYPE_GREATER, TYPE_BETWEEN, TYPE_CONTAINS, TYPE_LESSER_EQUAL,
TYPE_GREATER_EQUAL, TYPE_ORDER_ASC/DESC) to treat $values as an array
unconditionally: remove is_array checks and else branches, iterate $values
directly (use $values[0] ?? null for single-value semantics or when selecting
first element), and simplify parameter construction (e.g., for BETWEEN use
$values[0] and $values[1] with null coalescing); keep special handling for
attribute === 'time' and preserve param naming (param_ . $paramCounter++) and
usage of formatDateTime/formatParamValue and
escapeIdentifier/validateAttributeName.
In `@src/Usage/Adapter/Database.php`:
- Around line 189-199: Add scalar type validation and matching PHPDoc for the
TYPE_LESSER_EQUAL and TYPE_GREATER_EQUAL branches: mirror the pattern used by
TYPE_GREATER, TYPE_LESSER, and TYPE_BETWEEN by adding a PHPDoc like /** `@var`
bool|float|int|string $value */ above the extraction and add a runtime guard
(e.g., if (!is_scalar($value) && !is_bool($value)) continue; or use
is_scalar($value)) before calling DatabaseQuery::lessThanEqual($attribute,
$value) and DatabaseQuery::greaterThanEqual($attribute, $value) so non-scalar
inputs are ignored and TypeErrors are avoided.
🧹 Nitpick comments (1)
src/Usage/Adapter/Database.php (1)
52-59: Silence unused parameter warning.PHPMD flags
$idas unused; consider renaming to$_idor suppressing for clarity.
| switch ($method) { | ||
| case Query::TYPE_EQUAL: | ||
| $this->validateAttributeName($attribute); | ||
| $escapedAttr = $this->escapeIdentifier($attribute); | ||
|
|
||
| // Support arrays of values (produce IN (...) ) or single value equality | ||
| if (is_array($values)) { | ||
| $inParams = []; | ||
| foreach ($values as $value) { | ||
| $paramName = 'param_' . $paramCounter++; | ||
| if ($attribute === 'time') { | ||
| $inParams[] = "{{$paramName}:DateTime64(3)}"; | ||
| $params[$paramName] = $this->formatDateTime($value); | ||
| } else { | ||
| $inParams[] = "{{$paramName}:String}"; | ||
| $params[$paramName] = $this->formatParamValue($value); | ||
| } | ||
| } | ||
|
|
||
| if (count($inParams) === 1) { | ||
| $filters[] = "{$escapedAttr} = " . $inParams[0]; | ||
| } else { | ||
| $filters[] = "{$escapedAttr} IN (" . implode(', ', $inParams) . ")"; | ||
| } | ||
| } else { | ||
| $paramName = 'param_' . $paramCounter++; | ||
| if ($attribute === 'time') { | ||
| $filters[] = "{$escapedAttr} = {{$paramName}:DateTime64(3)}"; | ||
| $params[$paramName] = $this->formatDateTime($values); | ||
| } else { | ||
| $filters[] = "{$escapedAttr} = {{$paramName}:String}"; | ||
| $params[$paramName] = $this->formatParamValue($values); | ||
| } | ||
| } | ||
| break; | ||
|
|
||
| case Query::TYPE_LESSER: | ||
| $this->validateAttributeName($attribute); | ||
| $escapedAttr = $this->escapeIdentifier($attribute); | ||
| $paramName = 'param_' . $paramCounter++; | ||
| $value = is_array($values) && !empty($values) ? $values[0] : $values; | ||
| if ($attribute === 'time') { | ||
| $filters[] = "{$escapedAttr} < {{$paramName}:DateTime64(3)}"; | ||
| $params[$paramName] = $this->formatDateTime($value); | ||
| } else { | ||
| $filters[] = "{$escapedAttr} < {{$paramName}:String}"; | ||
| $params[$paramName] = $this->formatParamValue($value); | ||
| } | ||
| break; | ||
|
|
||
| case Query::TYPE_GREATER: | ||
| $this->validateAttributeName($attribute); | ||
| $escapedAttr = $this->escapeIdentifier($attribute); | ||
| $paramName = 'param_' . $paramCounter++; | ||
| $value = is_array($values) && !empty($values) ? $values[0] : $values; | ||
| if ($attribute === 'time') { | ||
| $filters[] = "{$escapedAttr} > {{$paramName}:DateTime64(3)}"; | ||
| $params[$paramName] = $this->formatDateTime($value); | ||
| } else { | ||
| $filters[] = "{$escapedAttr} > {{$paramName}:String}"; | ||
| $params[$paramName] = $this->formatParamValue($value); | ||
| } | ||
| break; | ||
|
|
||
| case Query::TYPE_BETWEEN: | ||
| $this->validateAttributeName($attribute); | ||
| $escapedAttr = $this->escapeIdentifier($attribute); | ||
| $paramName1 = 'param_' . $paramCounter++; | ||
| $paramName2 = 'param_' . $paramCounter++; | ||
| // Between has two values | ||
| $value1 = is_array($values) && isset($values[0]) ? $values[0] : $values; | ||
| $value2 = is_array($values) && isset($values[1]) ? $values[1] : $values; | ||
| if ($attribute === 'time') { | ||
| $paramType = 'DateTime64(3)'; | ||
| $filters[] = "{$escapedAttr} BETWEEN {{$paramName1}:{$paramType}} AND {{$paramName2}:{$paramType}}"; | ||
| $params[$paramName1] = $this->formatDateTime($value1); | ||
| $params[$paramName2] = $this->formatDateTime($value2); | ||
| } else { | ||
| $filters[] = "{$escapedAttr} BETWEEN {{$paramName1}:String} AND {{$paramName2}:String}"; | ||
| $params[$paramName1] = $this->formatParamValue($value1); | ||
| $params[$paramName2] = $this->formatParamValue($value2); | ||
| } | ||
| break; | ||
|
|
||
|
|
||
|
|
||
| case Query::TYPE_ORDER_DESC: | ||
| $this->validateAttributeName($attribute); | ||
| $escapedAttr = $this->escapeIdentifier($attribute); | ||
| $orderBy[] = "{$escapedAttr} DESC"; | ||
| break; | ||
|
|
||
| case Query::TYPE_ORDER_ASC: | ||
| $this->validateAttributeName($attribute); | ||
| $escapedAttr = $this->escapeIdentifier($attribute); | ||
| $orderBy[] = "{$escapedAttr} ASC"; | ||
| break; | ||
|
|
||
| case Query::TYPE_CONTAINS: | ||
| $this->validateAttributeName($attribute); | ||
| $escapedAttr = $this->escapeIdentifier($attribute); | ||
| $inParams = []; | ||
| foreach ($values as $value) { | ||
| $paramName = 'param_' . $paramCounter++; | ||
| if ($attribute === 'time') { | ||
| $inParams[] = "{{$paramName}:DateTime64(3)}"; | ||
| $params[$paramName] = $this->formatDateTime($value); | ||
| } else { | ||
| $inParams[] = "{{$paramName}:String}"; | ||
| $params[$paramName] = $this->formatParamValue($value); | ||
| } | ||
| } | ||
| if (!empty($inParams)) { | ||
| $filters[] = "{$escapedAttr} IN (" . implode(', ', $inParams) . ")"; | ||
| } | ||
| break; | ||
|
|
||
| case Query::TYPE_LESSER_EQUAL: | ||
| $this->validateAttributeName($attribute); | ||
| $escapedAttr = $this->escapeIdentifier($attribute); | ||
| $paramName = 'param_' . $paramCounter++; | ||
| if ($attribute === 'time') { | ||
| $filters[] = "{$escapedAttr} <= {{$paramName}:DateTime64(3)}"; | ||
| $params[$paramName] = $this->formatDateTime(is_array($values) ? ($values[0] ?? null) : $values); | ||
| } else { | ||
| $filters[] = "{$escapedAttr} <= {{$paramName}:String}"; | ||
| $params[$paramName] = $this->formatParamValue(is_array($values) ? ($values[0] ?? null) : $values); | ||
| } | ||
| break; | ||
|
|
||
| case Query::TYPE_GREATER_EQUAL: | ||
| $this->validateAttributeName($attribute); | ||
| $escapedAttr = $this->escapeIdentifier($attribute); | ||
| $paramName = 'param_' . $paramCounter++; | ||
| if ($attribute === 'time') { | ||
| $filters[] = "{$escapedAttr} >= {{$paramName}:DateTime64(3)}"; | ||
| $params[$paramName] = $this->formatDateTime(is_array($values) ? ($values[0] ?? null) : $values); | ||
| } else { | ||
| $filters[] = "{$escapedAttr} >= {{$paramName}:String}"; | ||
| $params[$paramName] = $this->formatParamValue(is_array($values) ? ($values[0] ?? null) : $values); | ||
| } | ||
| break; |
There was a problem hiding this comment.
CodeQL‑reported unreachable branches (CI blocker).
Query::getValues() always returns an array, so is_array($values) branches are always true and the else paths are unreachable. This is causing CodeQL failures. Consider simplifying to treat $values as an array everywhere (e.g., $values[0] ?? null) and remove the dead branches.
Example simplification
- if (is_array($values)) {
- $inParams = [];
- foreach ($values as $value) {
+ $inParams = [];
+ foreach ($values as $value) {
$paramName = 'param_' . $paramCounter++;
...
- }
-
- if (count($inParams) === 1) {
- $filters[] = "{$escapedAttr} = " . $inParams[0];
- } else {
- $filters[] = "{$escapedAttr} IN (" . implode(', ', $inParams) . ")";
- }
- } else {
- ...
- }
+ }
+ if (count($inParams) === 1) {
+ $filters[] = "{$escapedAttr} = " . $inParams[0];
+ } else {
+ $filters[] = "{$escapedAttr} IN (" . implode(', ', $inParams) . ")";
+ }🧰 Tools
🪛 GitHub Actions: CodeQL
[error] 1040-1040: Else branch is unreachable because previous condition is always true.
[error] 1122-1122: Parameter #1 $dateTime of method Utopia\Usage\Adapter\ClickHouse::formatDateTime() expects DateTime|string|null, mixed given.
[error] 1139-1139: Else branch is unreachable because ternary operator condition is always true.
[error] 1142-1142: Else branch is unreachable because ternary operator condition is always true.
[error] 1152-1152: Else branch is unreachable because ternary operator condition is always true.
[error] 1155-1155: Else branch is unreachable because ternary operator condition is always true.
🤖 Prompt for AI Agents
In `@src/Usage/Adapter/ClickHouse.php` around lines 1016 - 1157, The CodeQL issue
is that Query::getValues() always returns an array, so all is_array($values)
conditional branches are dead; update the switch handling in ClickHouse.php
(cases like Query::TYPE_EQUAL, TYPE_LESSER, TYPE_GREATER, TYPE_BETWEEN,
TYPE_CONTAINS, TYPE_LESSER_EQUAL, TYPE_GREATER_EQUAL, TYPE_ORDER_ASC/DESC) to
treat $values as an array unconditionally: remove is_array checks and else
branches, iterate $values directly (use $values[0] ?? null for single-value
semantics or when selecting first element), and simplify parameter construction
(e.g., for BETWEEN use $values[0] and $values[1] with null coalescing); keep
special handling for attribute === 'time' and preserve param naming (param_ .
$paramCounter++) and usage of formatDateTime/formatParamValue and
escapeIdentifier/validateAttributeName.
| case Query::TYPE_LESSER_EQUAL: | ||
| if (!empty($values)) { | ||
| $value = $values[0]; | ||
| $dbQueries[] = DatabaseQuery::lessThanEqual($attribute, $value); | ||
| } | ||
| break; | ||
| case Query::TYPE_GREATER_EQUAL: | ||
| if (!empty($values)) { | ||
| $value = $values[0]; | ||
| $dbQueries[] = DatabaseQuery::greaterThanEqual($attribute, $value); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's find the DatabaseQuery class location
find . -type f -name "*.php" | xargs grep -l "class DatabaseQuery" | head -5Repository: utopia-php/usage
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Let's check what DatabaseQuery is imported from in the target file
head -50 src/Usage/Adapter/Database.php | grep -E "^use|^import|DatabaseQuery"Repository: utopia-php/usage
Length of output: 339
🏁 Script executed:
#!/bin/bash
# Now let's find the Query class in vendor
find vendor -type f -path "*/Utopia/Database/Query.php" 2>/dev/null | head -3Repository: utopia-php/usage
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Let's check if vendor exists and look at the structure
ls -la vendor/ 2>/dev/null | head -20
# Also check the size of Database.php
wc -l src/Usage/Adapter/Database.phpRepository: utopia-php/usage
Length of output: 94
🏁 Script executed:
#!/bin/bash
# Read the full Database.php file to understand context
cat -n src/Usage/Adapter/Database.phpRepository: utopia-php/usage
Length of output: 14951
🏁 Script executed:
#!/bin/bash
# Let's find the Utopia\Usage\Query class
find . -type f -name "*.php" | xargs grep -l "class Query" | grep -v vendor | head -10Repository: utopia-php/usage
Length of output: 109
🏁 Script executed:
#!/bin/bash
# Read the Query class
cat -n src/Usage/Query.phpRepository: utopia-php/usage
Length of output: 9531
🏁 Script executed:
#!/bin/bash
# Let's check for test files related to Database adapter
find tests -type f -name "*Database*" -o -name "*database*" | head -10Repository: utopia-php/usage
Length of output: 96
🏁 Script executed:
#!/bin/bash
# Read the Database test file
wc -l tests/Usage/Adapter/DatabaseTest.php
head -100 tests/Usage/Adapter/DatabaseTest.phpRepository: utopia-php/usage
Length of output: 1521
🏁 Script executed:
#!/bin/bash
# Find UsageBase trait
find tests -type f -name "*.php" | xargs grep -l "trait UsageBase"Repository: utopia-php/usage
Length of output: 85
🏁 Script executed:
#!/bin/bash
# Read UsageBase trait
wc -l tests/Usage/UsageBase.php
cat -n tests/Usage/UsageBase.phpRepository: utopia-php/usage
Length of output: 9432
🏁 Script executed:
#!/bin/bash
# Check git log for recent changes to Database.php or references to lessThanEqual/greaterThanEqual
git log --oneline -20 -- src/Usage/Adapter/Database.php 2>/dev/null | head -20Repository: utopia-php/usage
Length of output: 245
🏁 Script executed:
#!/bin/bash
# Show the details of that commit
git show a89db31 --statRepository: utopia-php/usage
Length of output: 1743
🏁 Script executed:
#!/bin/bash
# Check the actual diff for the recent commit to see what was changed
git show a89db31 -- src/Usage/Adapter/Database.php | head -150Repository: utopia-php/usage
Length of output: 4999
🏁 Script executed:
#!/bin/bash
# Check git status and recent commits to see if there are any related fixes or issues
git log --all --oneline | grep -i "lessThanEqual\|greaterThanEqual\|type.*guard\|scalar" | head -10Repository: utopia-php/usage
Length of output: 245
Add type validation for lessThanEqual/greaterThanEqual to match pattern of other comparison operators.
Unlike TYPE_GREATER (line 165), TYPE_LESSER (line 172), and TYPE_BETWEEN (line 181) which include PHPDoc type hints specifying bool|float|int|string, the TYPE_LESSER_EQUAL and TYPE_GREATER_EQUAL cases lack this documentation and runtime validation. To maintain consistency and prevent TypeError when non-scalar values are passed, add explicit scalar type guards:
Suggested fix
case Query::TYPE_LESSER_EQUAL:
if (!empty($values)) {
+ /** `@var` bool|float|int|string $value */
$value = $values[0];
$dbQueries[] = DatabaseQuery::lessThanEqual($attribute, $value);
}
break;
case Query::TYPE_GREATER_EQUAL:
if (!empty($values)) {
+ /** `@var` bool|float|int|string $value */
$value = $values[0];
$dbQueries[] = DatabaseQuery::greaterThanEqual($attribute, $value);
}
break;🧰 Tools
🪛 GitHub Actions: CodeQL
[error] 192-192: Parameter #2 $value of static method Utopia\Database\Query::lessThanEqual() expects bool|float|int|string, mixed given.
[error] 198-198: Parameter #2 $value of static method Utopia\Database\Query::greaterThanEqual() expects bool|float|int|string, mixed given.
🤖 Prompt for AI Agents
In `@src/Usage/Adapter/Database.php` around lines 189 - 199, Add scalar type
validation and matching PHPDoc for the TYPE_LESSER_EQUAL and TYPE_GREATER_EQUAL
branches: mirror the pattern used by TYPE_GREATER, TYPE_LESSER, and TYPE_BETWEEN
by adding a PHPDoc like /** `@var` bool|float|int|string $value */ above the
extraction and add a runtime guard (e.g., if (!is_scalar($value) &&
!is_bool($value)) continue; or use is_scalar($value)) before calling
DatabaseQuery::lessThanEqual($attribute, $value) and
DatabaseQuery::greaterThanEqual($attribute, $value) so non-scalar inputs are
ignored and TypeErrors are avoided.
…Equal query types
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/Usage/Adapter/ClickHouse.php`:
- Around line 506-526: formatDateTime currently doesn't return for types other
than null, DateTime, or string (causing a TypeError); update formatDateTime to
cover other inputs by: if $dateTime is an integer (timestamp) create a DateTime
from that timestamp and return formatted string, else throw a clear
InvalidArgumentException (or Exception) that includes the input type/value so
the method always returns a string or fails explicitly; reference the
formatDateTime method to locate where to add the integer-handling block and the
final exception.
In `@src/Usage/Adapter/Database.php`:
- Around line 229-237: The DatabaseQuery::orderDesc() calls in the getByPeriod
method's authorization-skip closure are missing the required string $attribute
argument; update those calls to pass 'time' (e.g.,
DatabaseQuery::orderDesc('time')) so ordering matches ClickHouse and tests;
apply the same fix to the other DatabaseQuery::orderDesc() invocations in this
file (the other authorization closure further down) and ensure the calls inside
convertQueriesToDatabase/getAuthorization()->skip closures consistently pass
'time'.
🧹 Nitpick comments (3)
src/Usage/Adapter/ClickHouse.php (2)
601-604: Redundant type check.The
$tagsparameter has type hintarray(line 581), sois_array($tags)is always true. This defensive check is harmless but unnecessary.
1141-1160: Simplify redundantis_arraychecks.
Query::getValues()always returns an array, making theis_array($values)checks redundant. The current structure is confusing because$singleValueis assigned conditionally but used unconditionally.♻️ Cleaner implementation
case Query::TYPE_LESSER_EQUAL: $this->validateAttributeName($attribute); $escapedAttr = $this->escapeIdentifier($attribute); $paramName = 'param_' . $paramCounter++; + $singleValue = $values[0] ?? null; if ($attribute === 'time') { - if (is_array($values)) { - /** `@var` \DateTime|string|null $singleValue */ - $singleValue = $values[0] ?? null; - } $filters[] = "{$escapedAttr} <= {{$paramName}:DateTime64(3)}"; $params[$paramName] = $this->formatDateTime($singleValue); } else { - if (is_array($values)) { - /** `@var` bool|float|int|string $singleValue */ - $singleValue = $values[0] ?? null; - } $filters[] = "{$escapedAttr} <= {{$paramName}:String}"; $params[$paramName] = $this->formatParamValue($singleValue); } break;src/Usage/Adapter/Database.php (1)
52-58: Silence PHPMD unused-parameter warning for required override.
getColumnDefinition(string $id)is required by the abstract base but not used here. Add a suppression to avoid noisy analysis warnings.🧹 Suggested tweak
- /** - * Get column definition for Database adapter (not used, but required by SQL parent) - */ + /** + * Get column definition for Database adapter (not used, but required by SQL parent) + * + * `@SuppressWarnings`(PHPMD.UnusedFormalParameter) + */ protected function getColumnDefinition(string $id): string
| public function getByPeriod(string $metric, string $period, array $queries = []): array | ||
| { | ||
| /** @var array<Document> $result */ | ||
| $result = $this->db->getAuthorization()->skip(function () use ($queries, $metric, $period) { | ||
| $dbQueries = $this->convertQueriesToDatabase($queries); | ||
| $dbQueries[] = DatabaseQuery::equal('metric', [$metric]); | ||
| $dbQueries[] = DatabaseQuery::equal('period', [$period]); | ||
| $dbQueries[] = DatabaseQuery::orderDesc(); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the DatabaseQuery class definition
fd -t f "DatabaseQuery.php" -o -t f "*Query.php" | head -20Repository: utopia-php/usage
Length of output: 176
🏁 Script executed:
# Search for orderDesc method definition
rg -n "function orderDesc|public.*orderDesc" -A 3Repository: utopia-php/usage
Length of output: 505
🏁 Script executed:
# Check usage patterns of orderDesc throughout codebase
rg "orderDesc\(" -B 2 -A 1 | head -50Repository: utopia-php/usage
Length of output: 1884
Fix missing required parameter in DatabaseQuery::orderDesc() calls.
DatabaseQuery::orderDesc() requires a string $attribute parameter, but is called without arguments. This will cause a runtime error. Pass 'time' to match the pattern in ClickHouse.php and tests:
Required fix
- $dbQueries[] = DatabaseQuery::orderDesc();
+ $dbQueries[] = DatabaseQuery::orderDesc('time');Also applies to: lines 247–256
🤖 Prompt for AI Agents
In `@src/Usage/Adapter/Database.php` around lines 229 - 237, The
DatabaseQuery::orderDesc() calls in the getByPeriod method's authorization-skip
closure are missing the required string $attribute argument; update those calls
to pass 'time' (e.g., DatabaseQuery::orderDesc('time')) so ordering matches
ClickHouse and tests; apply the same fix to the other DatabaseQuery::orderDesc()
invocations in this file (the other authorization closure further down) and
ensure the calls inside convertQueriesToDatabase/getAuthorization()->skip
closures consistently pass 'time'.
- Introduced logCounter method to log individual usage counter metrics with upsert behavior. - Added logBatchCounter method to log multiple usage counter metrics in batch, allowing for individual entries without aggregation. - Updated logBatch method to accept a batch size parameter for better control over batch processing. - Enhanced Usage class to support new logging methods and batch size functionality. - Implemented tests for new methods, including scenarios for batch sizes, counter behavior, and metrics with tags.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/Usage/Adapter/ClickHouse.php`:
- Around line 737-750: The buildInsertColumns() method appends $attribute['$id']
which is currently typed as mixed; update the loop in buildInsertColumns() to
assert and handle the type (e.g., use is_string($attribute['$id']) and either
cast to string before appending or skip/throw on invalid types) so the returned
array contains only strings and matches the declared return type; reference the
getAttributes() iteration and the '$id' key in the attribute to locate where to
add the type check and handling.
- Around line 706-717: The loop uses $attribute['$id'] without guaranteeing it's
a string before calling getColumnType; cast and validate it first: retrieve the
raw id from getAttributes() into a local variable, assert/cast it to string
(e.g., (string)$rawId) and optionally handle missing/empty ids (skip or throw)
before calling getColumnType($attrId), and then use that typed $attrId for
building $attrKey, $queryParams and $valuePlaceholders so getColumnType always
receives a string.
- Around line 850-863: The CodeQL/static-analysis warnings come from untyped
array accesses in logBatchCounter (and similarly in logBatch); after
validateMetricsBatch has run, add explicit type assertions/casts when extracting
$metricData elements in logBatchCounter (cast $metric to string, $value to
int/float as appropriate, $tags to array, $period to string/int, and $tenant to
string|null when using resolveTenantFromMetric) before passing them into
buildInsertValuesForMetric; update the extraction lines in logBatchCounter (and
the analogous lines in logBatch) to perform these casts so static analysis
recognizes the narrowed types while leaving validation logic unchanged.
🧹 Nitpick comments (4)
tests/Usage/UsageBase.php (2)
302-330: Remove unused variables$aggCountand$counterCount.These variables are assigned but never used in assertions. Either remove them or add assertions to verify the count behavior matches expectations.
Proposed fix
$aggSum = $this->usage->sumByPeriod('agg-vs-counter', '1h'); - $aggCount = $this->usage->countByPeriod('agg-vs-counter', '1h'); // Clear for counter test $this->usage->purge(DateTime::now()); // Log same counter metric 3 times (last one wins) $this->assertTrue($this->usage->logCounter('counter-vs-agg', 10, '1h', [])); $this->assertTrue($this->usage->logCounter('counter-vs-agg', 20, '1h', [])); $this->assertTrue($this->usage->logCounter('counter-vs-agg', 30, '1h', [])); $counterSum = $this->usage->sumByPeriod('counter-vs-agg', '1h'); - $counterCount = $this->usage->countByPeriod('counter-vs-agg', '1h');
173-195: Assertions always pass - consider strengthening.Both
testLessThanEqualQueryandtestGreaterThanEqualQueryuseassertGreaterThanOrEqual(0, count($results))which always passes. If the intent is only to verify no exceptions are thrown, consider usingassertIsArray($results)instead for clarity.src/Usage/Adapter/Database.php (2)
93-140:$batchSizeparameter is unused - consider implementing or documenting.The
logBatchmethod ignores the$batchSizeparameter and processes all metrics in a single upsert operation. While this may be acceptable for the Database adapter, it differs from the ClickHouse adapter which respects the batch size for chunked inserts. Consider either:
- Implementing batch chunking for consistency
- Adding a comment explaining why batching isn't needed for this adapter
388-407: Consider using batch deletion for better performance.The
purgemethod deletes documents one-by-one in a loop. For large datasets, this could be slow. Consider using a batch deletion approach if the underlying database supports it.
|
|
||
| // Add attributes dynamically - must include ALL attributes in schema order | ||
| foreach ($this->getAttributes() as $attribute) { | ||
| $attrId = $attribute['$id']; | ||
|
|
||
| $attrKey = $attrId . ($paramCounter > 0 ? '_' . $paramCounter : ''); | ||
| $type = $this->getColumnType($attrId); | ||
|
|
||
| // Use the value from map, or null if not present | ||
| $queryParams[$attrKey] = $attributeMap[$attrId] ?? null; | ||
| $valuePlaceholders[] = '{' . $attrKey . ':' . $type . '}'; | ||
| } |
There was a problem hiding this comment.
Add type assertion for $attrId before calling getColumnType.
Proposed fix
// Add attributes dynamically - must include ALL attributes in schema order
foreach ($this->getAttributes() as $attribute) {
- $attrId = $attribute['$id'];
+ $attrId = $attribute['$id'];
+ if (!is_string($attrId)) {
+ continue;
+ }
$attrKey = $attrId . ($paramCounter > 0 ? '_' . $paramCounter : '');
$type = $this->getColumnType($attrId);🧰 Tools
🪛 GitHub Actions: CodeQL
[error] 712-712: Parameter #1 $id of method Utopia\Usage\Adapter\ClickHouse::getColumnType() expects string, mixed given.
🤖 Prompt for AI Agents
In `@src/Usage/Adapter/ClickHouse.php` around lines 706 - 717, The loop uses
$attribute['$id'] without guaranteeing it's a string before calling
getColumnType; cast and validate it first: retrieve the raw id from
getAttributes() into a local variable, assert/cast it to string (e.g.,
(string)$rawId) and optionally handle missing/empty ids (skip or throw) before
calling getColumnType($attrId), and then use that typed $attrId for building
$attrKey, $queryParams and $valuePlaceholders so getColumnType always receives a
string.
| private function buildInsertColumns(): array | ||
| { | ||
| $insertColumns = ['id']; | ||
|
|
||
| foreach ($this->getAttributes() as $attribute) { | ||
| $insertColumns[] = $attribute['$id']; | ||
| } | ||
|
|
||
| if ($this->sharedTables) { | ||
| $insertColumns[] = 'tenant'; | ||
| } | ||
|
|
||
| return $insertColumns; | ||
| } |
There was a problem hiding this comment.
Add type assertion for attribute ID to fix return type.
$attribute['$id'] is typed as mixed, causing the return type mismatch. Add a type check.
Proposed fix
private function buildInsertColumns(): array
{
$insertColumns = ['id'];
foreach ($this->getAttributes() as $attribute) {
- $insertColumns[] = $attribute['$id'];
+ $id = $attribute['$id'];
+ $insertColumns[] = is_string($id) ? $id : '';
}
if ($this->sharedTables) {
$insertColumns[] = 'tenant';
}
return $insertColumns;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private function buildInsertColumns(): array | |
| { | |
| $insertColumns = ['id']; | |
| foreach ($this->getAttributes() as $attribute) { | |
| $insertColumns[] = $attribute['$id']; | |
| } | |
| if ($this->sharedTables) { | |
| $insertColumns[] = 'tenant'; | |
| } | |
| return $insertColumns; | |
| } | |
| private function buildInsertColumns(): array | |
| { | |
| $insertColumns = ['id']; | |
| foreach ($this->getAttributes() as $attribute) { | |
| $id = $attribute['$id']; | |
| $insertColumns[] = is_string($id) ? $id : ''; | |
| } | |
| if ($this->sharedTables) { | |
| $insertColumns[] = 'tenant'; | |
| } | |
| return $insertColumns; | |
| } |
🧰 Tools
🪛 GitHub Actions: CodeQL
[error] 749-749: Method Utopia\Usage\Adapter\ClickHouse::buildInsertColumns() should return array but returns array<int, mixed>.
🤖 Prompt for AI Agents
In `@src/Usage/Adapter/ClickHouse.php` around lines 737 - 750, The
buildInsertColumns() method appends $attribute['$id'] which is currently typed
as mixed; update the loop in buildInsertColumns() to assert and handle the type
(e.g., use is_string($attribute['$id']) and either cast to string before
appending or skip/throw on invalid types) so the returned array contains only
strings and matches the declared return type; reference the getAttributes()
iteration and the '$id' key in the attribute to locate where to add the type
check and handling.
| foreach ($metricsBatch as $metricData) { | ||
| $period = $metricData['period'] ?? Usage::PERIOD_1H; | ||
| $metric = $metricData['metric']; | ||
| $value = $metricData['value']; | ||
| $tags = (array) ($metricData['tags'] ?? []); | ||
|
|
||
| // Build values for this metric | ||
| $tenant = $this->sharedTables ? $this->resolveTenantFromMetric($metricData) : null; | ||
| $result = $this->buildInsertValuesForMetric($metric, $value, $period, $tags, $tenant, $paramCounter); | ||
|
|
||
| $queryParams = array_merge($queryParams, $result['queryParams']); | ||
| $valueClauses[] = '(' . implode(', ', $result['valuePlaceholders']) . ')'; | ||
| $paramCounter++; | ||
| } |
There was a problem hiding this comment.
Add type assertions to satisfy static analysis after validation.
The batch processing in logBatchCounter extracts values from $metricData without type assertions, causing CodeQL errors. Since validateMetricsBatch already validates the types, add explicit casts to narrow the types for static analysis.
Proposed fix
foreach ($metricsBatch as $metricData) {
$period = $metricData['period'] ?? Usage::PERIOD_1H;
$metric = $metricData['metric'];
$value = $metricData['value'];
$tags = (array) ($metricData['tags'] ?? []);
+
+ // Type assertions (validated by validateMetricsBatch)
+ $metric = is_string($metric) ? $metric : '';
+ $value = is_int($value) ? $value : 0;
+ $period = is_string($period) ? $period : Usage::PERIOD_1H;
// Build values for this metric
$tenant = $this->sharedTables ? $this->resolveTenantFromMetric($metricData) : null;
$result = $this->buildInsertValuesForMetric($metric, $value, $period, $tags, $tenant, $paramCounter);Also applies to logBatch at lines 966-974.
🧰 Tools
🪛 GitHub Actions: CodeQL
[error] 858-858: Parameter #1 $metric of method Utopia\Usage\Adapter\ClickHouse::buildInsertValuesForMetric() expects string, mixed given.
[error] 858-858: Parameter #2 $value of method Utopia\Usage\Adapter\ClickHouse::buildInsertValuesForMetric() expects int, mixed given.
[error] 858-858: Parameter #3 $period of method Utopia\Usage\Adapter\ClickHouse::buildInsertValuesForMetric() expects string, mixed given.
[error] 858-858: Parameter #4 $tags of method Utopia\Usage\Adapter\ClickHouse::buildInsertValuesForMetric() expects array<string, mixed>, array given.
🤖 Prompt for AI Agents
In `@src/Usage/Adapter/ClickHouse.php` around lines 850 - 863, The
CodeQL/static-analysis warnings come from untyped array accesses in
logBatchCounter (and similarly in logBatch); after validateMetricsBatch has run,
add explicit type assertions/casts when extracting $metricData elements in
logBatchCounter (cast $metric to string, $value to int/float as appropriate,
$tags to array, $period to string/int, and $tenant to string|null when using
resolveTenantFromMetric) before passing them into buildInsertValuesForMetric;
update the extraction lines in logBatchCounter (and the analogous lines in
logBatch) to perform these casts so static analysis recognizes the narrowed
types while leaving validation logic unchanged.
…remove ClickHouseBatchTest
… Adapter and Database classes
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/Usage/Adapter/ClickHouse.php`:
- Around line 683-688: When PERIOD_INF is used the code sets $time to null which
yields an empty string param and lets ClickHouse coerce to 1970 implicitly;
instead set a deterministic sentinel datetime for $timestamp when
Usage::PERIOD_INF is selected. Update the block around $now/$time/$timestamp
(used with Usage::PERIODS and formatDateTime) to assign a concrete sentinel
string (e.g. '1970-01-01 00:00:00.000') and pass that through
$this->formatDateTime so $timestamp is never null, ensuring partition key
behavior is explicit and deterministic.
| $now = new \DateTime(); | ||
| $time = $period === Usage::PERIOD_INF | ||
| ? null | ||
| : $now->format(Usage::PERIODS[$period]); | ||
| $timestamp = $time !== null ? $this->formatDateTime($time) : null; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && find . -type f -name "ClickHouse.php" 2>/dev/null | head -5Repository: utopia-php/usage
Length of output: 42
🏁 Script executed:
# First, let's locate the file
find . -type f -name "ClickHouse.php" -path "*/Usage/Adapter/*" 2>/dev/nullRepository: utopia-php/usage
Length of output: 94
🏁 Script executed:
# Let's search from the repo root
git ls-files | grep -i clickhouseRepository: utopia-php/usage
Length of output: 131
🏁 Script executed:
# Read the relevant file section
head -n 700 src/Usage/Adapter/ClickHouse.php | tail -n 50Repository: utopia-php/usage
Length of output: 1626
🏁 Script executed:
# Get more context around lines 683-688
sed -n '670,700p' src/Usage/Adapter/ClickHouse.phpRepository: utopia-php/usage
Length of output: 978
🏁 Script executed:
# Search for table schema or CREATE TABLE statement
rg -i "CREATE TABLE|table_schema" src/Usage/Adapter/ClickHouse.php | head -20Repository: utopia-php/usage
Length of output: 202
🏁 Script executed:
# Search for the column mapping/attribute map where timestamp is used
sed -n '680,750p' src/Usage/Adapter/ClickHouse.phpRepository: utopia-php/usage
Length of output: 2342
🏁 Script executed:
# Search for how attributes/columns are mapped in the insert
rg -A 30 "attributeMap.*=" src/Usage/Adapter/ClickHouse.php | head -60Repository: utopia-php/usage
Length of output: 1246
🏁 Script executed:
# Find the complete CREATE TABLE statement
rg -B 5 -A 50 "CREATE TABLE IF NOT EXISTS" src/Usage/Adapter/ClickHouse.php | head -150Repository: utopia-php/usage
Length of output: 2819
🏁 Script executed:
# Also check if there are any migration files or schema definitions
find . -type f \( -name "*.php" -o -name "*.sql" \) | xargs grep -l "time.*DateTime" 2>/dev/null | grep -i schemaRepository: utopia-php/usage
Length of output: 42
🏁 Script executed:
# Search for column type definitions or schema initialization
rg "time.*DateTime|PARTITION BY" src/Usage/Adapter/ClickHouse.phpRepository: utopia-php/usage
Length of output: 438
🏁 Script executed:
# Search for tests related to PERIOD_INF or this code path
rg -i "PERIOD_INF|period_inf" tests/ src/Repository: utopia-php/usage
Length of output: 263
🏁 Script executed:
# Check if there's error handling around NULL inserts
rg -B 3 -A 3 "try|catch|Exception.*time|Exception.*NULL" src/Usage/Adapter/ClickHouse.php | head -50Repository: utopia-php/usage
Length of output: 1853
🏁 Script executed:
# Search for how the query is actually executed with parameters
rg -B 5 -A 10 "query.*INSERT.*valuePlaceholders|execute.*INSERT" src/Usage/Adapter/ClickHouse.php | head -80Repository: utopia-php/usage
Length of output: 42
🏁 Script executed:
# Search for formatParamValue function
rg -A 20 "function formatParamValue|formatParamValue.*=" src/Usage/Adapter/ClickHouse.phpRepository: utopia-php/usage
Length of output: 571
🏁 Script executed:
# Also check the full picture - how parameters are formatted in the HTTP request
rg -B 10 -A 15 "formatParamValue" src/Usage/Adapter/ClickHouse.php | head -80Repository: utopia-php/usage
Length of output: 3381
🏁 Script executed:
# Check the Usage.php file for PERIODS mapping
rg -B 3 -A 10 "PERIODS.*=" src/Usage/Usage.phpRepository: utopia-php/usage
Length of output: 394
🏁 Script executed:
# Search for getColumnType function
rg -A 30 "function getColumnType" src/Usage/Adapter/ClickHouse.phpRepository: utopia-php/usage
Length of output: 1070
🏁 Script executed:
# Also check how 'time' is defined as an attribute
rg -B 5 -A 15 "getAttributes.*=|'time'.*DateTime" src/Usage/Adapter/ClickHouse.php | head -60Repository: utopia-php/usage
Length of output: 42
🏁 Script executed:
# Search for where 'time' attribute is defined or set
rg -B 5 -A 5 "'time'" src/Usage/Adapter/ClickHouse.php | grep -A 5 -B 5 "required\|type"Repository: utopia-php/usage
Length of output: 42
🏁 Script executed:
# Check the attributes array or getAttributes implementation
rg -B 10 -A 30 "function getAttributes|protected.*attributes" src/Usage/Adapter/ClickHouse.php | head -100Repository: utopia-php/usage
Length of output: 42
🏁 Script executed:
# Look for attribute initialization in parent class or base
fd "Adapter.php" src/ --type fRepository: utopia-php/usage
Length of output: 81
🏁 Script executed:
# Check the base Adapter class
cat -n src/Usage/Adapter.phpRepository: utopia-php/usage
Length of output: 3630
🏁 Script executed:
# Search for 'time' attribute definition specifically
rg -B 3 -A 3 "'time'" src/Usage/Adapter.phpRepository: utopia-php/usage
Length of output: 42
🏁 Script executed:
# Search for how attributes are defined - looking for 'metric', 'value', 'period', 'time', 'tags'
rg -B 5 -A 20 "metric.*value.*period|'metric'|'period'|'tags'" src/Usage/Adapter/ClickHouse.php | head -100Repository: utopia-php/usage
Length of output: 3660
🏁 Script executed:
# Look at how 'metric', 'value', etc. are mapped to columns
rg -B 3 -A 3 "getAttribute.*time|'time'.*required" src/Usage/Adapter/ClickHouse.phpRepository: utopia-php/usage
Length of output: 42
🏁 Script executed:
# Search for where the schema attributes are hardcoded or predefined
rg -B 5 -A 20 "buildInsertColumns|columnDefs.*=" src/Usage/Adapter/ClickHouse.php | head -80Repository: utopia-php/usage
Length of output: 2698
🏁 Script executed:
# Search for getAttributes implementation
rg -A 30 "function getAttributes" src/Usage/Adapter/ClickHouse.phpRepository: utopia-php/usage
Length of output: 42
🏁 Script executed:
# Check if 'time', 'metric', 'value', 'period', 'tags' are hardcoded or predefined
rg -B 10 "columns.*metric.*value|columns.*=.*\[\]" src/Usage/Adapter/ClickHouse.php | head -80Repository: utopia-php/usage
Length of output: 294
🏁 Script executed:
# Look for where columns array is built and check if time is always included
rg -B 20 "time.*DateTime64|columns\[\].*time" src/Usage/Adapter/ClickHouse.php | head -100Repository: utopia-php/usage
Length of output: 2718
🌐 Web query:
ClickHouse DateTime64 non-nullable column empty string parameter NULL behavior insert
💡 Result:
If you insert an empty string ('') into a non-nullable DateTime64 column, ClickHouse does not treat that as NULL. What happens depends on how you’re inserting (format/settings):
- CSV input: empty fields are treated as the column default value when
input_format_csv_empty_as_default=1(this is true by default)—so an empty field forDateTime64becomes its default (typically the “zero” datetime) rather thanNULL. [2] - JSONEachRow input: empty strings are only replaced with defaults if you enable
input_format_json_empty_as_default=1. Otherwise, the empty string is just a string and parsing can fail. [3] - Actual
NULLinto a non-nullable column throws an error unless you enableinput_format_null_as_default=1, in which caseNULLis replaced by the column’s default. [1][6]
How to handle “empty string means missing” safely
Option A (recommended): make the column nullable
ts Nullable(DateTime64(3))Then send NULL when you mean missing.
Option B: convert empty string to NULL, then choose a fallback
INSERT INTO t (ts)
VALUES (coalesce(toDateTime64OrNull(nullif({p:String}, ''), 3), toDateTime64('1970-01-01 00:00:00', 3)));toDateTime64OrNull returns NULL on invalid/empty input. [4]
Option C: convert empty/invalid directly to the default
INSERT INTO t (ts)
VALUES (toDateTime64OrDefault({p:String}, 3));toDateTime64OrDefault returns the provided default, or 1970-01-01 00:00:00.000 if you don’t provide one. [5]
Sources: [1] Format setting input_format_null_as_default [2] CSV input_format_csv_empty_as_default [3] JSON input_format_json_empty_as_default [4] toDateTime64OrNull [5] toDateTime64OrDefault [6] non-nullable NULL insert error
Set a deterministic timestamp for PERIOD_INF instead of relying on implicit ClickHouse coercion.
The time column is non-nullable DateTime64(3) and used as a partition key. When PERIOD_INF is used, $timestamp is set to null, which becomes an empty string when sent as a parameter. ClickHouse then silently coerces the empty string to its default datetime (1970-01-01 00:00:00), making the behavior implicit and fragile.
Use an explicit sentinel timestamp (e.g., '1970-01-01 00:00:00.000') or reconsider the column design:
Proposed fix (sentinel timestamp)
- $time = $period === Usage::PERIOD_INF
- ? null
- : $now->format(Usage::PERIODS[$period]);
- $timestamp = $time !== null ? $this->formatDateTime($time) : null;
+ $time = $period === Usage::PERIOD_INF
+ ? '1970-01-01 00:00:00.000'
+ : $now->format(Usage::PERIODS[$period]);
+ $timestamp = $this->formatDateTime($time);🤖 Prompt for AI Agents
In `@src/Usage/Adapter/ClickHouse.php` around lines 683 - 688, When PERIOD_INF is
used the code sets $time to null which yields an empty string param and lets
ClickHouse coerce to 1970 implicitly; instead set a deterministic sentinel
datetime for $timestamp when Usage::PERIOD_INF is selected. Update the block
around $now/$time/$timestamp (used with Usage::PERIODS and formatDateTime) to
assign a concrete sentinel string (e.g. '1970-01-01 00:00:00.000') and pass that
through $this->formatDateTime so $timestamp is never null, ensuring partition
key behavior is explicit and deterministic.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/Usage/Adapter/Database.php`:
- Around line 482-486: The setSharedTables method currently calls itself causing
infinite recursion; change the internal call from
$this->setSharedTables($sharedTables) to call the underlying database adapter
$this->db->setSharedTables($sharedTables) (ensure $this->db is the correct
property and initialized) and then return $this to preserve fluent interface in
the setSharedTables method.
🧹 Nitpick comments (3)
src/Usage/Adapter/Database.php (3)
93-93: Unused$batchSizeparameter.The
$batchSizeparameter is declared but never used. Either implement batching logic to process documents in chunks of$batchSize, or remove the parameter to avoid confusion.Option 1: Implement batching
public function logBatch(array $metrics, int $batchSize = 1000): bool { $this->db->getAuthorization()->skip(function () use ($metrics, $batchSize) { $documentsById = []; // ... existing aggregation logic ... - if (!empty($documents)) { - $this->db->upsertDocumentsWithIncrease($this->collection, 'value', $documents); - } + if (!empty($documents)) { + foreach (array_chunk($documents, $batchSize) as $batch) { + $this->db->upsertDocumentsWithIncrease($this->collection, 'value', $batch); + } + } });
188-188: Unused$batchSizeparameter (same aslogBatch).Apply the same batching fix as suggested for
logBatch(), or remove the parameter.
388-407: Purge implementation is functional but could be optimized.The current approach deletes documents one-by-one in a loop. While this works correctly, it may be slow for large datasets. Consider using a bulk delete operation if available in the Utopia\Database library.
| public function setSharedTables(bool $sharedTables): self | ||
| { | ||
| $this->setSharedTables($sharedTables); | ||
| return $this; | ||
| } |
There was a problem hiding this comment.
Critical: Infinite recursion in setSharedTables().
Line 484 calls $this->setSharedTables($sharedTables) instead of $this->db->setSharedTables($sharedTables), causing a stack overflow at runtime.
Required fix
public function setSharedTables(bool $sharedTables): self
{
- $this->setSharedTables($sharedTables);
+ $this->db->setSharedTables($sharedTables);
return $this;
}🤖 Prompt for AI Agents
In `@src/Usage/Adapter/Database.php` around lines 482 - 486, The setSharedTables
method currently calls itself causing infinite recursion; change the internal
call from $this->setSharedTables($sharedTables) to call the underlying database
adapter $this->db->setSharedTables($sharedTables) (ensure $this->db is the
correct property and initialized) and then return $this to preserve fluent
interface in the setSharedTables method.
Usage
Automatically construct ID using md5 hash
Automatically aggregate metric in DB library using upsertDocumentWithIncrease and in ClickHouse using
summingMergeTreeengine and final select to make sure the aggregation happensDatabase adapter (utopia-php/database)
ClickHouse adapter
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.